Change smart picker behavior - #130
Conversation
| // on a selected cell and mid-edit (e.g. "asdf /"); _smartPickerReplace | ||
| // then drives the "/" removal on insert. | ||
| document.addEventListener('keydown', function(e) { | ||
| var key = e.key || (e.originalEvent && e.originalEvent.key); |
There was a problem hiding this comment.
I believe e.originalEvent only exists on jquery wrapped events
This is a native listener and e is already the KeyboardEvent
This fallback never runs Just var key = e.key
| // Not editing (selected cell): append to the committed cell | ||
| // text via the value path (no formula operators, so no "+"); | ||
| // drop a trailing "/" trigger if present. | ||
| var cell = this.api.asc_getCellInfo && this.api.asc_getCellInfo(); | ||
| var cur = (cell && cell.asc_getText && cell.asc_getText()) || ''; | ||
| if (cur.slice(-1) === '/') { | ||
| cur = cur.slice(0, -1); | ||
| } | ||
| this.api.asc_insertInCell(cur + data, Asc.c_oAscPopUpSelectorType.None); |
There was a problem hiding this comment.
The comment says "no formula operators so no +", but that's not really the issue here.
As I investigate
asc_getText() internally calls getValueForEdit() (WorksheetView.js:13669) which it returns the same value shown in the formula bar. For formula cells that's the formula itself (for example, =SUM(A1:A10)) not the evaluated result
As a result cur + data becomes something like:
=SUM(A1:A10)https://example.com (the formula source with the =) when you pass that to asc_insertInCell the SDK sees a string starting with = and tries to parse it as a formula
The url appended after it makes it syntactically invalid → the cell becomes #NAME?. The original formula is gone.
Even for non formula cells the behavior isn't ideal! If the cell contains 42 concatenation produces 42https://... converting the numeric value into plain text instead of preserving it as a number
Suggestion: if the cell is empty (cur === '') just insert data directly.
If the cell isn't empty, it's not obvious what the expected behavior should be. Should we replace the existing value append to it or reject the operation? It would be better to decide that explicitly instead of silently producing incorrect results for non-empty cells.
…mething Addresses the second review comment on PR #130. asc_getText() returns the formula-bar value, so cur + data wrote "=SUM(A1:A10)https://..." into a formula cell, which the SDK then failed to parse -- #NAME?, formula gone. The reviewer also noted values fare no better: "42" became "42https://..." and stopped being a number. Both are silent data loss. The previous guard only caught formulas. Appending, replacing and rejecting are all defensible for a non-empty cell, and the review asked for that decision to be explicit rather than implied, so: reject any non-empty cell and name the two unambiguous paths -- an empty cell, or editing the cell, where the branch above inserts at the cursor. An empty cell now inserts the link alone rather than concatenating onto ''. The string is renamed to txtCellNotEmpty since it is no longer only about formulas, and moved to keep the locale file sorted. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
f8cbbc5 to
ba48759
Compare
…nsert
The "/" trigger cancelled its own keystroke. sdkjs inserts printable characters
from CDocument.OnKeyPress (EnterText), which preventDefault on keydown
suppresses, so "/" could not be typed after a space at all: the menu reopened on
every attempt and the character never reached the document. insertLink then
"removed" that "/" with pluginMethod_InputText('', '/'), which loops
emulateKeyDownApi(8) once per character of textReplace -- deleting the real
character before the caret, usually the space the user had just typed.
The trigger no longer cancels anything, and the flow now follows Nextcloud's
Text app, which drives the same interaction with @tiptap/suggestion configured
as {char: '/', allowedPrefixes: [' ']}: the "/" is written to the document, each
further character is written and narrows the list, space or a second "/" ends
the match because tiptap's query class is [^\s/], backspacing over the trigger
closes the menu, and accepting an entry replaces "/" plus the query the way
tiptap's command() calls deleteRange(range). Up/Down/Enter/Tab/Escape are the
only keys taken from the editor, which is why the listener moved to the capture
phase -- sdkjs binds its own handler to #area_id, so a bubble listener runs too
late to stop the caret moving. Focus stays in the document, so the highlight
uses the class bootstrap's :focus rule already styles rather than moving focus.
Addresses the rest of the review on PR #130:
- Spreadsheet: four statements sat after an unconditional return, so
_smartPickerReplace was never set and the documented "/" removal did not
exist. Removed; the replacement text now comes from the shared session.
- _smartPickerSlashArtificial was never assigned true, leaving the cancel-path
cleanup dead in all three editors. Dropped.
- SmartPickerMenu leaked a Common.UI.Menu per keystroke: Menu registers with
Menu.Manager on construction and only unregisters from remove(), which
hide() never calls. Every later hideAll() walked the accumulated list.
- Provider icon_url reached MenuItem's unescaped <img src="<%= iconImg %>">.
It is now checked against a scheme allowlist; these urls come from whichever
Nextcloud apps registered a provider.
- The pending-request flag was trusted for two minutes, so an unrelated
insertLink in that window took the backspace path. Cut to 60s and cleared on
any keystroke in the editor, which proves the host's picker is gone.
- txtAnyLink could not be translated: _applyLocalization builds
Common.Views.SmartPickerMenu as a plain object, which the module then
replaced. It uses the _.extend pattern the controllers use, and the strings
are in each editor's locale file.
- Missing AGPL headers on the two files this branch added.
- e.key is no longer assumed to be a string; it is absent on some synthetic and
IME events, and throwing from a document keydown listener breaks typing.
The trigger, the session state machine and the pending-request tracking were
copy-pasted into three controllers and had already drifted -- the dead block
existed only in the spreadsheet. They now live in Common.Utils.SmartPicker;
only the insertion itself, which genuinely differs per editor, stays behind.
Unit tests cover slashCanTrigger, sanitizeIconUrl and the pending-request
tracking. They run under `node --test` and, once the harness repair lands,
in test/unit-tests/common/index.html.
Refs #122
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
The setup module publishes assert/expect and stubs the ambient Common.* state, and it sat in the same flat require array as the suites that read them. RequireJS makes no promise about the order of independent siblings in one array, so nest the call and let the setup resolve first -- the same pattern the vendor globals above already use. The SmartPicker suite registration moves to #130, which is where the module and its test file come from. This branch now stands on its own: 12 tests, no failures. Assisted-by: ClaudeCode:claude-opus-5
|
Review in progress (started earlier but found some things that need additional checks before posting) |
89d25ac to
7488dce
Compare
The setup module publishes assert/expect and stubs the ambient Common.* state, and it sat in the same flat require array as the suites that read them. RequireJS makes no promise about the order of independent siblings in one array, so nest the call and let the setup resolve first -- the same pattern the vendor globals above already use. The SmartPicker suite registration moves to #130, which is where the module and its test file come from. This branch now stands on its own: 12 tests, no failures. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
|
Reviewed asap as requested first Method: AI-assisted (single + multi-angle) + manual testing. Verdict: 👀 some lookups are recommended to the Author
I share my report below, beeing new in the codebase, i've left criticality/priority entirely to your judgment below. 📄 Full report here (click to expand)What's working well 🙌The core happy path is solid: typing Things I think are worth a look before merge 👀1. 🟠HIGH - The 2. 🟡MEDIUM - Clicking away doesn't actually truly close the caret menu feature, even though it visually looks like it does. 🔍 Reproduced live in documenteditor. Typed 3. ⚪LOW - A keystroke in the narrow gap between picking an option and Nextcloud's dialog opening leaves the trigger stranded. 🔍 Reproduced live in documenteditor. Picking an option closes the caret session immediately, before the reply arrives. Any keystroke landing in that gap — even an unrelated one, like a stray 4. ⚪LOW - The menu doesn't reposition on window resize, despite its own code comment saying it should. 🔍 Reproduced live in documenteditor. Opened the caret menu, resized the browser window without typing anything further — the menu stayed anchored at its original position instead of following the resize, matching a doc-comment in Things the AI found reading the code, that I couldn't reproduce through the UIWant to be upfront that these didn't hold up under live testing, even though the code pattern itself looks worth a defensive fix: 5. Two different entry points (the toolbar button and the caret menu) could in theory misattribute each other's replies. The AI's static review flagged that 6. A cell being edited via 7. Switching the active cell while a request is pending, in theory, could misdirect the result. Spreadsheet-specific, tested in spreadsheeteditor. This one also came from the AI's static read, not something I'd spotted myself. I tried to test whether clicking a different cell while Nextcloud's picker dialog is open could cause the link to land in the wrong place, but that dialog turns out to be a proper focus-trapping modal — neither clicks nor keyboard reach the sheet underneath while it's open, so I couldn't actually get into this state through normal use. Smaller thingsThese came out of the AI's static read of the code rather than something I verified by hand:
Just sharing, not asking for changesAlso from the AI's static read, minor enough that I didn't chase these further:
Reviewed Assisted by Claude Sonnet 5 Thanks for all the work on this, happy to pair on any of the above if useful 🙌 |
test/unit-tests/common/index.html could not run at all. Every path in it, and in
the two test files it loads, pointed into a "web-apps — копия" directory that is
not in this repository, and requirejs aborts the whole run on the first 404. The
suite has been dead long enough for the rest of it to rot behind that.
Working outwards from there:
- Paths now resolve inside this checkout. baseUrl was '../../apps/', which from
test/unit-tests/common/ is test/apps/ -- a directory that has never existed.
- mocha.setup() no longer passes ignoreLeaks. Removed in mocha 4, and setup()
calls every key as a method, so an unknown one throws "self[opt] is not a
function" before a single test registers.
- chai 5 is ESM only ("type": "module", no UMD build), so requirejs' classic
script tag dies on `export`. The page imports it as a module and registers it
under the id the test files already use, leaving define(['chai']) and
require('chai') untouched.
- jquery, underscore and backbone load before the tests, and are published to
window. The components read them as globals without declaring them as
dependencies, and underscore's UMD build registers as AMD without leaving a
global behind, so requirejs was free to evaluate a component first.
- Module ids ending in .js resolve against the page rather than baseUrl, so the
ids inside the test files lost their extension and the ones in index.html
kept theirs.
That leaves the Button suite, which was failing on its own terms:
- .andSelf() was removed in jQuery 3; it is .addBack() now.
- Common.UI.Scaling.currentRatio(), Common.Locale.isCurrentLanguageRtl() and
Common.NotificationCenter are read at render time. The real modules pull in
'core' and the whole application bootstrap, which is the opposite of a unit
test, so common.js stubs the three.
12 tests, no failures. Serve the repository over http and open
test/unit-tests/common/index.html -- requirejs cannot load modules from file://,
where Chrome gives every URL its own opaque origin.
No product code is touched.
The runner also registers the SmartPicker unit test, whose module and test file
arrive with Euro-Office#130. Until that merges, requirejs 404s on it and
aborts the run, so this has to land second.
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…vior Refs #122 - Rename button caption and tooltip from "Smart Picker" / "Ask Nextcloud Assistant" to "Add from Nextcloud" in all three editors - On toolbar button click, insert "/" at cursor position before opening the picker, so the flow matches the Notes app slash-command UX - When the user selects a result, the inserted "/" is replaced by the hyperlink via pluginMethod_InputText backspace + add_Hyperlink - Replace sparkle icon with a plain "+" in the same stroke style as other toolbar icons (btn-inserthyperlink template) Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…t at cursor Refs #122 Works together with the Euro-Office/eurooffice-nextcloud branch "fix/issue-122-smart-picker-behavior" (host-side integration). Both branches are required for the feature to work. - Show the "Add from Nextcloud" toolbar button only while connected to a Nextcloud server: the button now defaults to hidden (visible:false) and is toggled by a new setSmartPickerAvailable host command (api.js + Gateway). The "Ask Nextcloud Assistant" context-menu item keeps its existing setAssistantAvailable gating. - Trigger the picker by typing "/" in the editor body, layout-independent (e.key). In spreadsheets the keydown listener runs in the capture phase so it also fires while a cell is being edited. - Replace the inserted/typed "/" with the selected result on success and leave it in place on cancel (new setSmartPickerCancel host command). - Restore editor focus after insert and cancel via edit:complete. - Spreadsheets: insert the link as text (cell hyperlinks are whole-cell), using isCellEdited to pick pluginMethod_InputText at the cursor while editing (re-focusing the cell input) vs asc_insertInCell on a selected cell; strip the "/" trigger and never fall into the formula path. - Use a dedicated "+" icon (btn-nc-add / btn-big-nc-add) drawn in the standard 1px toolbar stroke and drop the old btn-nc-assistant sparkle. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…s pickers Typing "/" after a space or newline opens an editor-native menu at the cursor listing what Nextcloud offers to insert. Choosing an entry hands off to Nextcloud's own picker for that provider, so only the provider choice is drawn by us. The pickers themselves carry behaviour that is invisible from the outside -- minimum search lengths, per-provider result shapes, icon resolution -- and reimplementing them means rediscovering all of it one defect at a time. The provider list is pushed in by the host rather than fetched here, because a provider is only openable where its picker component is registered, and that is a fact about the host page. The list arrives as an object: Gateway relays commands through jQuery's trigger(), which spreads an array into separate handler arguments, so a bare array would arrive as its first element. Positioning is per editor. Writer and Presentation anchor on #id_target_cursor, the caret element the drawing document moves; #area_id_parent is not the caret, since sdkjs places that IME wrapper at caretBottom plus a chain of IME offsets. The spreadsheet has no text caret unless a cell is being edited inline, so it anchors on the active cell via asc_getActiveCellCoord(), as its own popups do. "/" is accepted using the same rule as Nextcloud's editors, which configure Tribute.js with requireLeadingSpace: it fires at the start of the text or after a single whitespace character. Non-character keys are ignored when tracking the previous keystroke, or a German keyboard's Shift+7 would hide the space before it. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
"Add from Nextcloud" opened a dialog we built ourselves. Point it at Nextcloud's own Smart Picker instead, with no provider preselected so it shows the provider list. The caret menu stays as it is: that exists to keep the "/" flow inside the editor, whereas this button is the "give me the full Nextcloud picker" entry point. Deletes AssistantDialog.js, the action list we maintained in it, and its 16 locale strings per editor. Its insertion code was the part worth keeping, so that moves to Common.Utils.AssistantInsert -- including the retry around pluginMethod_PasteHtml, which is re-entrancy guarded and silently drops a second insertion. Adds an insertAssistantResult command so the host can hand back a result to insert. Nextcloud's Assistant form has no way of its own to write into our document; it accepts actionButtons, so the connector adds an "Insert into document" button whose output comes back through here and is pasted as HTML, keeping headings, lists and emphasis. Common.Assistant is now only used to record whether the Assistant is available; the request channel it wraps has no callers left. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
Removes what this branch added and no longer uses: Common.Assistant in full (its request/getTaskTypes/run/cancel had no callers, and setAvailable only stored a value nothing read), Gateway's requestAssistant and setAssistantResult, and api.js's _setAssistantResult with its export and doc line. Left alone deliberately: setAssistantAvailable exists in origin/main, and the handler that consumes it lives in DocumentHolder.js, which this branch never touched. That is what shows "Ask Nextcloud Assistant" in the context menu, so it keeps working without any of our code -- verified in the built bundle, where the command survives and ours are gone. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…mething Addresses the second review comment on PR #130. asc_getText() returns the formula-bar value, so cur + data wrote "=SUM(A1:A10)https://..." into a formula cell, which the SDK then failed to parse -- #NAME?, formula gone. The reviewer also noted values fare no better: "42" became "42https://..." and stopped being a number. Both are silent data loss. The previous guard only caught formulas. Appending, replacing and rejecting are all defensible for a non-empty cell, and the review asked for that decision to be explicit rather than implied, so: reject any non-empty cell and name the two unambiguous paths -- an empty cell, or editing the cell, where the branch above inserts at the cursor. An empty cell now inserts the link alone rather than concatenating onto ''. The string is renamed to txtCellNotEmpty since it is no longer only about formulas, and moved to keep the locale file sorted. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…nsert
The "/" trigger cancelled its own keystroke. sdkjs inserts printable characters
from CDocument.OnKeyPress (EnterText), which preventDefault on keydown
suppresses, so "/" could not be typed after a space at all: the menu reopened on
every attempt and the character never reached the document. insertLink then
"removed" that "/" with pluginMethod_InputText('', '/'), which loops
emulateKeyDownApi(8) once per character of textReplace -- deleting the real
character before the caret, usually the space the user had just typed.
The trigger no longer cancels anything, and the flow now follows Nextcloud's
Text app, which drives the same interaction with @tiptap/suggestion configured
as {char: '/', allowedPrefixes: [' ']}: the "/" is written to the document, each
further character is written and narrows the list, space or a second "/" ends
the match because tiptap's query class is [^\s/], backspacing over the trigger
closes the menu, and accepting an entry replaces "/" plus the query the way
tiptap's command() calls deleteRange(range). Up/Down/Enter/Tab/Escape are the
only keys taken from the editor, which is why the listener moved to the capture
phase -- sdkjs binds its own handler to #area_id, so a bubble listener runs too
late to stop the caret moving. Focus stays in the document, so the highlight
uses the class bootstrap's :focus rule already styles rather than moving focus.
Addresses the rest of the review on PR #130:
- Spreadsheet: four statements sat after an unconditional return, so
_smartPickerReplace was never set and the documented "/" removal did not
exist. Removed; the replacement text now comes from the shared session.
- _smartPickerSlashArtificial was never assigned true, leaving the cancel-path
cleanup dead in all three editors. Dropped.
- SmartPickerMenu leaked a Common.UI.Menu per keystroke: Menu registers with
Menu.Manager on construction and only unregisters from remove(), which
hide() never calls. Every later hideAll() walked the accumulated list.
- Provider icon_url reached MenuItem's unescaped <img src="<%= iconImg %>">.
It is now checked against a scheme allowlist; these urls come from whichever
Nextcloud apps registered a provider.
- The pending-request flag was trusted for two minutes, so an unrelated
insertLink in that window took the backspace path. Cut to 60s and cleared on
any keystroke in the editor, which proves the host's picker is gone.
- txtAnyLink could not be translated: _applyLocalization builds
Common.Views.SmartPickerMenu as a plain object, which the module then
replaced. It uses the _.extend pattern the controllers use, and the strings
are in each editor's locale file.
- Missing AGPL headers on the two files this branch added.
- e.key is no longer assumed to be a string; it is absent on some synthetic and
IME events, and throwing from a document keydown listener breaks typing.
The trigger, the session state machine and the pending-request tracking were
copy-pasted into three controllers and had already drifted -- the dead block
existed only in the spreadsheet. They now live in Common.Utils.SmartPicker;
only the insertion itself, which genuinely differs per editor, stays behind.
Unit tests cover slashCanTrigger, sanitizeIconUrl and the pending-request
tracking. They run under `node --test` and, once the harness repair lands,
in test/unit-tests/common/index.html.
Refs #122
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
… adds
The four files this branch adds carried the Ascensio System SIA header, copied
from neighbouring upstream files to satisfy the "every file needs a licence
header" rule. That got the licence right and the copyright holder wrong: it
credited Ascensio for code they did not write, and asserted their Section 7
additional terms -- the non-infringement warranty exclusion and the CC-BY-SA
clause on GUI elements -- over Nextcloud-authored work. Those are Ascensio's
terms to place on Ascensio's code.
Replaced with the SPDX header this repository already uses for exactly this
case, in apps/spreadsheeteditor/main/app/view/CheckBoxSettingsDialog.js:
/*!
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH or an Nextcloud affiliate company and Euro-Office contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
AGPL-3.0-or-later matches LICENSE.txt, so the licence itself is unchanged. The
/*! form is deliberate -- terser keeps bang comments and strips plain ones, so
the notice survives into the built bundle.
Affects only files this branch adds:
apps/common/main/lib/util/AssistantInsert.js
apps/common/main/lib/util/SmartPicker.js
apps/common/main/lib/view/SmartPickerMenu.js
test/unit-tests/common/main/lib/util/SmartPicker.js
Upstream files this branch modifies keep their original Ascensio headers. The
two SVGs it adds stay bare, as every other asset in the repository is.
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
The suite file arrives with this branch, so its registration belongs here too rather than in the harness fix (#194), which now stands on its own. Note that the runner itself only works once #194 lands -- every path in this file still points at a directory that is not in the repository. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…icker
PENDING_TIMEOUT was one minute, which is inside the time an ordinary
interaction takes. Picking a provider starts the record; the reply consumes
it and deletes the "/query" the user typed. In between, the host's picker is
a modal in front of the editor, so nothing clears the record -- and expiring
there does not fail safe: insertLink still inserts the link, but consume()
has already returned null, so the trigger text is left in the document beside
it.
Measured against a running editor, same clicks in the same order:
11 s from picking a provider to confirming -> "Hello /prohttp://..." became
"Hello http://..." correct
77 s -> "Hello /prohttp://..." wrong
Raised to ten minutes rather than removing the check, because the check is
still the documented backstop for a host that neither answers nor cancels.
Expiring is only protective if the document moved without a keystroke, which
onActivity already covers for the keyboard, and the caret cannot move on its
own -- so a generous value gives up nothing that was actually being guarded.
The new test pins the failure: it passes at ten minutes and fails at one, and
the existing stale-request test is written relative to the constant, so it
keeps its meaning either way.
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
pluginMethod_InputText(text, textReplace) does not match textReplace against the document. It fires textReplace.length backspaces at the caret and inserts. Verified against a running editor: with "Hello world" and the caret at the end, asking it to replace "ZZZZ" -- a string that appears nowhere -- left "Hello w". So the deletion is only correct while the caret still sits right after the text the user typed, and co-editing reaches that state without the user doing anything: a remote change shifts this user's caret, and onActivity does not fire because a remote change is not a keystroke here. The trigger is also plain text in the shared document while the picker is open, so a co-author may tidy away what looks like a typo. Raising PENDING_TIMEOUT widened that window, which is what prompted looking at it. Failure is not symmetric: a stray "/query" left behind is cosmetic and the user can delete it, while eating four characters of someone else's sentence is data loss that syncs to everyone. So triggerStillThere permits the deletion only when the word before the caret still matches what was typed, and the link is inserted either way. It compares the query rather than the whole trigger because "/" is punctuation and so a word boundary: after "Hello /pro" the word part before the caret is "pro". A bare "/" expects "", which is what a caret after punctuation gives. asc_GetCurrentWord is exported by word/api.js only, so Presentation and Spreadsheet cannot be asked and keep the previous behaviour rather than lose the feature -- exporting it there would extend the guard to them unchanged. The guard also permits the deletion if the probe throws; a diagnostic must not stop an insertion. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
Four defects in one file, all found in review of #130 and each reproduced in a running editor before it was touched. "/" only worked once per document. lastKey stands in for "the character before the caret" and nothing ever reset it, so typing "/" left it as "/" for good and slashCanTrigger refused every later trigger -- another cell, another text box, another paragraph -- until a space happened to be typed first. Anything that moves the caret elsewhere now forgets it. Undefined is the permissive value, which is the deliberate trade: this cannot read the document the way tiptap can, and a menu one Escape away beats a trigger that silently stops working. Clicking away did not end the session. sdkjs handles pointerdown on its canvas overlay and cancels it, so no compatibility mousedown is ever synthesised: measured in a running Writer, a click in the document area fires pointerdown and click on #id_viewer_overlay and no mousedown at all. The mousedown-only listener therefore never ran. The list was hidden by the editor's own hideAll(), so it looked dismissed, while Enter much later still opened the host's picker for whatever was left highlighted. Now bound to both. A keystroke between picking a provider and the host's modal taking focus cancelled the request. It is not proof the picker never opened -- the reply is still ours, and the character landed in the document behind the trigger. Within a hand-off grace such a key extends the text the reply has to delete instead, so "/f" plus a stray "y" no longer survives in front of the inserted link. triggerStillThere refused the commonest flow there is. It expected asc_GetCurrentWord(-1) to answer "" for a bare "/", on the grounds that punctuation is a word boundary. Measured against a running Writer it answers "/" -- the boundary rule holds only once a query follows -- so type "/", pick the first entry, and the "/" stayed in the document in front of the link. Alongside those: sanitizeIconUrl now rejects "/\host/path", which parses as the protocol-relative "//host/path"; the sdkjs element ids this feature reaches for are named once in one map instead of spelled out at each use; the option carrying a jQuery object is called `holder` rather than `holderEl`; and install() gathers the Gateway wiring the three editor controllers each carried a drifting copy of, so the next fix lands in all three at once. Tests cover all of it, including the two DOM-level regressions, which run in the browser harness only. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
The comment on _position() said the container has to be re-aligned "again after any resize", and nothing ever listened for one: the menu stayed where it was opened while the document reflowed underneath it. Re-reading the anchor rather than re-clamping the old point is what a resize actually calls for -- the caret moves, and the cell the spreadsheet anchors to moves with it. Verified in a running Writer: shrinking the viewport moved the caret from x=431 to x=272 and the menu followed, where before it stayed at its original x. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
The retry loop waited two seconds for sdkjs's PasteHtml re-entrancy guard to clear and then called PasteHtml anyway -- the one call its own comment says that guard drops without a word. On a slow machine the Assistant's answer disappeared and nothing said so. Falls back to PasteText, which is not behind that guard and keeps the content at the cost of the formatting, and only warns when there is no text to fall back on. The guard element id and the retry budget are named rather than inline. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
Three things the three controllers each needed, and now share. The toolbar button registered nothing with the pending record and did not clear it either, so a request the host never answered nor cancelled -- its modal closed some way that told us nothing -- was still sitting there when the button's own reply arrived, and that insertion would delete a "/query" typed minutes ago somewhere else. It clears the record first. In the spreadsheet the deletion is aimed blind: triggerStillThere needs asc_GetCurrentWord, which is exported only in word/api.js, so it always answers "cannot tell" there. The cell the trigger was typed into is recorded with the request and compared when the reply lands; a selection reached by scrolling or by a co-author's change is not where the "/" was, so nothing of ours is deleted there. The Gateway wiring itself moves into Common.Utils.SmartPicker.install: it was three copies that had already drifted in comment wording and in which guards each carried. Only the anchor, the recorded cell and insertLink stay per editor. Also: txtCellNotEmpty comes from the locale file with no hardcoded English beside it, as every other string these controllers show does; and the edit-mode branch of insertLink now says why it deliberately has no empty-cell guard -- it writes at the cursor the user put there, where the branch below appends to a value it never read. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
This branch needed a plus for the new "Add from Nextcloud" button and took it by repointing btn-nc-assistant at it and deleting the sparkle, which also changed the icon of the context-menu item for the Assistant -- a different, untouched feature -- in all three editors. btn-nc-assistant is restored and the menu item points at it again; btn-big-nc-add is the button's own icon. The 24px btn-nc-add had no user left once the menu item stopped borrowing it, so it goes rather than sit in every sprite unused. Sprites regenerated with build/scripts/deploy-sprites.js, which makes the diff against main purely additive. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
The four mobile/index.html files changed in one hashed css filename each -- a local build's output committed alongside the source changes, with nothing of this feature in it. Back to main's content. They are generated and tracked, which is what makes that happen at all; #195 stops tracking them. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
7488dce to
b5ed4bd
Compare
|
@j-base64 @MonaAghili @moodyjmz — thanks for the thorough review, that was Needs the host side to work: Euro-Office/eurooffice-nextcloud#70. Ready for another look. |
|
Thanks for the rework 🙌, update reviewed against my original findings + a new live testing. Verdict: 👀 ✅ Mostly fixed, just suggest a quick look at a few new things surfaced live testing + 💡 UX comments in the full report below. Might be worth getting a casual or UX tester to try this feature.
📄 FULL REPORT, click to open (6 sections and 2 videos 🎞️)
|
…he cell Typing a long phrase into a cell and then "/" opened the list at the cell's left edge, most of a cell's width away from the "/" that had just been typed. The spreadsheet anchors to asc_getActiveCellCoord because it has no text caret -- but that is only true of a cell that is merely selected. A cell being edited inline has one, and that is where the trigger is. SmartPicker.cellEditorCaret reads it. Measuring the caret directly does not work: CellEditor._showCursor blinks #ce-cursor by toggling display, so getBoundingClientRect answers all zeros for half of every blink interval, and a menu that opened on the wrong half of a blink would land in the corner. The left/top/height _updateCursorPosition writes stay put through the blink, so those are read against #ce-canvas-outer's rect instead -- the caret is a position:absolute child of it. That container is display:none outside inline editing, which is also how "no cell is being edited" is recognised; the formula bar's separate "-menu" pair is left to the active-cell anchor. From the second review pass on #130. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
…e caret Following the caret through a resize was the previous attempt at this, and it does not survive a real drag: re-anchoring on every resize event made the menu jump between its two placements -- below the caret, and flipped above it once the list no longer fits -- and past a certain window width it left the screen entirely. Closing is simpler and is the honest answer to "the thing this was pointing at has moved". Nothing is lost either way: the "/" and its query are still in the document, so the list comes back where the caret now is. It has to be the session that ends, not just the menu that hides. A menu dismissed with the session still running is what let Enter much later still open the host's picker after a click-away, so the resize handler lives with the trigger and calls closeSession(); the menu's own _reanchor and its window binding go, and the anchor is read once at open. lastKey is deliberately left alone -- a resize moves the viewport, not the caret within the text, and clearing it would let the "/" already sitting in front of the caret trigger a second time. From the second review pass on #130. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
… read
Three repros in the review, one omission behind them: the reply path for
the toolbar button went straight to asc_insertHyperlink without looking
at the cell. A cell hyperlink is whole-cell -- WorksheetView's
setSelectionInfo("hyperlink") does setValue(text) over the range and
then setHyperlink -- so a cell holding text lost it to the raw url, and
a cell being edited had its whole text made the link's label, the user's
selection ignored.
It is guarded now the way the "/" path beside it already is: a cell with
committed text is refused with txtCellNotEmpty, and only an empty cell
is written.
A cell being edited is refused too, even when it reads empty, with its
own message. asc_getText() comes from the model (_getSelectionInfoCell
calls getValueForEdit) and cannot see the cell editor's uncommitted
buffer, so "empty" there means "nothing committed yet" -- and that
buffer is exactly what the insertion would overwrite. Saying "this cell
is not empty" would be describing something that did not happen, and the
way out of it is a different one.
The "/" path's own comment now says why it inserts plain text and
nothing else, since the same review asked why that flow yields a
clickable link in an empty cell and plain text after existing text. Both
follow from the whole-cell rule: "Hello " plus a link is not a thing a
spreadsheet can hold, and inserting a real hyperlink there would
silently drop the "Hello ".
From the second review pass on #130.
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
The review counted three names for one feature, depending on where you met it: nothing at all on the floating menu, "Smart Picker" on Nextcloud's own dialog, "Add from Nextcloud" on the toolbar button. The button carried the narrowest of the three, too -- "Any link" links to anything, Nextcloud or not. Smart Picker is the one to keep: it is what Nextcloud calls the feature, what the modal getLinkWithPicker opens is titled, and what the code and txtOpenFailed already say. The button follows, in all three editors. From the second review pass on #130. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
3 participants
This introduced calling smart picker by keystroke "/" anywhere in a doc, ppt, or xls.
Details are in #122
For testing you need connector app and web-apps on the following branches:
https://github.com/Euro-Office/eurooffice-nextcloud/tree/fix/issue-122-smart-picker-behavior
https://github.com/Euro-Office/web-apps/tree/fix/issue-122-smart-picker-behavior